Skip to content

fix(core): register sub-composition timelines after async build + lint rule#1638

Merged
WaterrrForever merged 2 commits into
mainfrom
fix/subcomp-timeline-registration
Jun 22, 2026
Merged

fix(core): register sub-composition timelines after async build + lint rule#1638
WaterrrForever merged 2 commits into
mainfrom
fix/subcomp-timeline-registration

Conversation

@WaterrrForever

Copy link
Copy Markdown
Collaborator

What

Fix sub-composition GSAP timelines that build asynchronously, plus a lint rule to catch the anti-pattern. Five registry code blocks register their timeline only after the document.fonts.ready build completes; a new core lint rule flags the early-registration pattern.

Why

When a composition builds its GSAP timeline inside document.fonts.ready (glyph metrics must be final before measuring), registering window.__timelines[id] before that async build leaves an empty timeline registered.

The runtime's sub-composition readiness gate treats "key present in __timelines" as "ready" and nests the child once. An empty timeline registered early gets nested empty and is never re-nested — so when the composition is used as a sub-composition, it renders blank (the animation that was added later inside fonts.ready is never picked up).

Standalone, the early registration is harmless (the timeline populates in place). The failure only surfaces when the block is mounted as a sub-composition.

How

Registry blocks — register after the build

registry/blocks/code-{diff,highlight,morph,scroll,typing}: move the window.__timelines[id] = tl assignment to the end of the fonts.ready callback, after the timeline is fully built, then call window.__hfForceTimelineRebind() so the runtime re-nests the now-populated timeline:

document.fonts.ready.then(function () {
  // …build the timeline…
  window.__timelines["code-diff"] = tl;            // register AFTER setup
  if (typeof window.__hfForceTimelineRebind === "function") {
    window.__hfForceTimelineRebind();               // re-nest now that we're populated
  }
});

Core lint — flag the anti-pattern

Add gsap_timeline_registered_before_async_build to packages/core/src/lint/rules/gsap.ts: warns when window.__timelines[...] is assigned before an async build boundary (document.fonts.ready / async callback), i.e. the registered timeline starts empty. Registration after the async boundary is the correct pattern and is skipped. Tests cover both the violating and the correct shapes.

Test plan

  • bun run test (core) — 647 passed / 5 skipped, including the new rule's tests.
  • lefthook pre-commit — oxlint / oxfmt / typecheck / commitlint clean.
  • The five registry blocks render correctly both standalone and mounted as a sub-composition (the sub-composition case is the one this fixes).

Independent of the workflow-skill PRs — branches off main, touches only packages/core lint + registry/blocks.

…t rule

When a composition builds its GSAP timeline inside document.fonts.ready (or any
async callback), registering window.__timelines[id] BEFORE the build leaves an
EMPTY timeline registered. The runtime's sub-composition readiness gate treats
"key present" as "ready" and nests the child once — an empty timeline gets
nested empty and is never re-nested, so the frame renders blank when used as a
sub-composition.

- registry/blocks/code-{diff,highlight,morph,scroll,typing}: register the
  timeline AFTER the fonts.ready build completes, then call
  window.__hfForceTimelineRebind() to re-nest now that it is populated.
- core lint: add rule gsap_timeline_registered_before_async_build to flag the
  early-registration anti-pattern, with tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small, well-scoped fix + lint guard. Read all 7 files end-to-end. Posting as COMMENT per the team's customer-partner-PR discipline; on merit, ready to stamp.

What I verified

The bug shape is real. Each of the five registry/blocks/code-* blocks has the same pre-fix pattern:

window.__timelines = window.__timelines || {};
var tl = gsap.timeline({ paused: true });
window.__timelines["code-X"] = tl;     // synchronous register
document.fonts.ready.then(function () {
  tl.from(...); buildEffect(tl, ...);  // build happens LATER
});

The runtime's sub-composition readiness gate (per the comment) treats "key present in __timelines" as "ready" and nests the child once. Between sync registration and fonts.ready resolution, that "once" nest captures an empty timeline. When fonts.ready later populates the local tl, the nested copy stays empty → blank render when mounted as a sub-composition.

The fix is right and applied byte-equivalently to all 5 blocks: the registration moves to the END of the fonts.ready callback, after all the tweens are added, then calls window.__hfForceTimelineRebind() (guarded by typeof ... === "function") to re-nest the populated timeline. Verified the same 11-line block at the end of each fonts.ready callback in code-diff / code-highlight / code-morph / code-scroll / code-typing.

The lint rule is correctly shaped:

  • Finds the first window.__timelines[ registration via content.search(/...__timelines\s*\[/).
  • Finds the first document.fonts.ready via content.search(/document\s*\.\s*fonts\s*\.\s*ready/).
  • Skips (no finding) when regIdx >= fontsReadyIdx (registration textually after the boundary = correct pattern).
  • Confirms the build is actually deferred — checks tail (the post-fonts.ready slice) for .to(, .from(, .fromTo(, or buildEffect( before flagging. Prevents false positives on empty fonts.ready callbacks.
  • stripJsComments(script.content) is called first, so a // document.fonts.ready mention in a comment can't fool the offset check.
  • Severity error matches the impact (blank sub-composition renders are a silent UX bug).

Tests cover both the violating and the correct shapes. ✓

Edge cases worth knowing (not blockers)

  • Multiple registrations + multiple async boundaries. content.search() returns FIRST match for each pattern. So a script with both a correct (post-fonts.ready) registration and a separate synchronous registration outside any async boundary would only fire on the first-match ordering. Probably fine for the immediate pattern being fixed (one registration per block) but worth knowing if registries gain multi-timeline blocks later. The current rule is a heuristic, not an AST analysis.
  • Finding message names the anti-pattern but not the offending key. Useful improvement for future iterations — findings.push({..., context: { id: <key> }}) — but not load-bearing for this PR.

CI

All checks still pending — just opened. No fail conclusions yet. Last-round CodeQL on the sibling Miao PRs is passing; expect the same here.

Stamp posture

Per team discipline on customer-partner PRs, stamp eligibility routes through <@U08E7PV788Z>. From my read this is ready — small, well-tested, clean fix + lint guard. Same hand-back to James as the other three.

Review by Jerrai

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — fix(core): register sub-composition timelines after async build + lint rule

Reviewed by Miga

Summary

Clean, well-scoped fix for a real sub-composition race condition. The root cause analysis is solid: window.__timelines[id] = tl registered synchronously before the document.fonts.ready callback populates the timeline, runtime readiness gate sees "key present" → nests once while empty → animation never renders in sub-composition context. The fix (move registration inside the callback, call __hfForceTimelineRebind after) and the lint rule to prevent regression are both well-motivated.

This PR is independent of the #1631/#1632/#1635 series — branches off main, touches only core lint + registry blocks. No overlap with the shared audio engine or script-driven architecture from #1635.

Correctness

The five registry block changes (code-diff, code-highlight, code-morph, code-scroll, code-typing) are structurally identical and correct. The typeof window.__hfForceTimelineRebind === "function" guard is good defensive coding for environments where the runtime hook isn't available.

The lint rule logic is sound:

  • Strips JS comments before analysis (via stripJsComments) — prevents false positives from commented-out code.
  • Uses string-index comparison (regIdx < fontsReadyIdx) to detect early registration — simple and effective for this pattern.
  • The tail regex confirms actual deferred build (tweens/buildEffect after fonts.ready) so it won't flag harmless cases where fonts.ready is referenced but no build happens there.

Observations (non-blocking)

  1. Lint rule scope is narrower than the comment suggests. The leading comment says "or any async callback" but the implementation only checks document.fonts.ready. That's fine for the five blocks this PR fixes (they all use fonts.ready), but if future blocks defer inside setTimeout, requestAnimationFrame, or fetch().then(), the rule won't catch them. Worth a // NOTE: in the code acknowledging this is intentionally scoped to fonts.ready for now, so a future author doesn't assume all async boundaries are covered.

  2. First-match semantics on multiple __timelines[ assignments. content.search() returns the index of the first match. If a script has two assignments — one early (bad) and one inside fonts.ready (good) — the rule correctly flags it because the first match is before fonts.ready. But if someone writes the correct assignment first and then has a redundant early one later in source order (unusual but possible in generated code), the rule would miss it. Unlikely to matter in practice, just flagging the model.

  3. Test coverage could be slightly broader. The two tests cover the core positive/negative cases well. Consider adding:

    • A case with synchronous build (no document.fonts.ready at all) → should NOT flag. (The fontsReadyIdx < 0 early-return handles this, but a test documents the intent.)
    • A case where __timelines is assigned before fonts.ready but no tweens appear after fonts.ready → should NOT flag. (The tail regex guard handles this, but again, a test documents it.)

Verdict

Looks good. The fix is correct, well-commented, and the lint rule prevents regression. The observations above are quality-of-life suggestions, none blocking. Recommend approval once CI is green.

Review by Miga

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-correction on my round-1: I said "CI still pending — just opened; no fail conclusions yet." That was true at check-time but is stale now — Magi correctly flagged that Test flipped to FAILURE while I was writing. Pulling the failure detail to record it on the PR thread, since both my GH review and Miga's read as "ready once CI green" and that's not the right state.

What's failing

Test check, in bun run --filter '!@hyperframes/producer' test, on @hyperframes/studio. 6 failures, all the same shape:

TypeError: (0 , commitGsapPositionFromDrag) is not a function

at packages/studio/src/hooks/gsapDragCommit.test.ts (lines 81, 100, 120, 168, 222, 393).

Why

Same upstream-squash-merge stacking issue I flagged on #1631 + #1635. This branch carries the renamed source files from those upstream squash-merge commits — gsapDragCommit.ts and the new gsapDragPositionCommit.ts both exist on the branch — but the test file on this branch still imports commitGsapPositionFromDrag from ./gsapDragCommit:

// packages/studio/src/hooks/gsapDragCommit.test.ts (on this branch)
import {
  commitGsapPositionFromDrag,
  ...
} from "./gsapDragCommit";   // <-- gone from this module

gsapDragCommit.ts on the branch no longer exports commitGsapPositionFromDrag (it moved to gsapDragPositionCommit.ts). Net: the import resolves to undefined, the test calls it, TypeError.

#1631 already carries a commit (c312ce70a "test(studio): import commitGsapPositionFromDrag from its actual module") that fixes the test import to point at the new module. So this is a missing-commit-on-branch problem, not a code defect this PR introduces.

Resolution paths

  • (a) Land #1631 first. Once #1631 merges to main, this branch rebased onto main picks up the test-import fix, CI goes green, no per-PR work needed here.
  • (b) Cherry-pick the test-import fix into this branch if the team wants #1638 unblocked independently. One-commit cherry-pick of c312ce70a (the test-import fix) would resolve the failure without waiting on #1631 ordering.
  • (c) Rebase onto current main + cherry-pick if #1631 lands but main moves past it.

Both (a) and (b) leave the code review verdict on #1638 itself unchanged — the sub-composition fix + lint guard are clean. This is purely a stacking-mechanics blocker.

Stamp posture

Unchanged from round-1 (routes through the team), now explicitly: CI gate is RED, do not stamp until one of the resolution paths above clears it.

Review by Jerrai

The function was split out into gsapDragPositionCommit.ts in #1605, but the
test kept importing it from ./gsapDragCommit, which no longer exports it —
yielding 'is not a function' at runtime. Import from the correct module.

Inherited main breakage (same fix as #1631/#1635); fixes the Test CI check on
this branch independently of merge order.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review — Round 2

Reviewed by Miga

Diff delta since last review

Compared against my prior review — the diff is unchanged. No new commits, no force-pushes, no code modifications. The two-commit stack is the same:

  1. The core fix (move window.__timelines[id] = tl inside document.fonts.ready.then(...) + __hfForceTimelineRebind() call) across all 5 code-* registry blocks.
  2. The test import fix (commitGsapPositionFromDraggsapDragPositionCommit).

CI status

All 47 checks pass. Every regression shard (8 shards), all perf suites, typecheck, lint, format, Windows cross-platform, CodeQL, CLI smoke, runtime contract tests — green across the board. No flakes, no skips on required checks.

Code assessment (unchanged from round 1, confirmed on re-read)

The fix is correct. The race condition is real and well-understood: registering window.__timelines[id] synchronously before the document.fonts.ready callback populates the timeline means the runtime's sub-composition readiness gate sees "key present" → "ready" and nests an empty timeline exactly once. Moving registration after the build completes, plus the __hfForceTimelineRebind() safety net, is the right pattern.

The lint rule is well-designed. gsap_timeline_registered_before_async_build catches the exact pattern — registration index before document.fonts.ready index, with a build-call guard in the tail — and the error message + fixHint are actionable. Comment-stripping via stripJsComments prevents false positives from commented-out code.

Tests cover both polarities. The two new test cases validate the lint rule fires on early registration and stays silent on correct (post-callback) registration.

Prior suggestions status

My round-1 review suggested two additional edge-case tests (multiple <script> tags with split registration/build, and registration inside a nested callback chain). These remain nice-to-haves — the current tests validate the core lint logic correctly, and neither suggested case maps to a pattern that exists in the actual registry blocks today. Not blocking.

Verdict

No new concerns. The fix is correct, the lint rule is solid, CI is fully green. This is ready to merge from a code perspective.

Review by Miga

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final stamp after Miguel authorization in Slack.

Reviewed current head 4099b4c0bc: the five registry/blocks/code-* timelines now register only after the document.fonts.ready build completes, then call window.__hfForceTimelineRebind() when available; the new lint rule catches early window.__timelines[id] registration before async timeline construction and has positive/negative tests. The follow-up import fix moves commitGsapPositionFromDrag to ./gsapDragPositionCommit, matching the module export. Live gh pr checks exits 0; GitHub is blocked only on review.

Verdict: APPROVE
Reasoning: The sub-composition readiness race is fixed at each affected registry block, regression coverage is present, and CI is green at the current head.

— Magi

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeat stamp per Miguel authorization in Slack. Current head remains 4099b4c0bc; live gh pr checks exits 0 and GitHub state is clean.

Verdict: APPROVE
Reasoning: Current head is already verified and has no remaining CI or review blockers.

— Magi

@WaterrrForever WaterrrForever merged commit 4db708e into main Jun 22, 2026
49 checks passed
@WaterrrForever WaterrrForever deleted the fix/subcomp-timeline-registration branch June 22, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants